-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LAMMPS non-unique molecule ID hotfix #1000
Conversation
…s for Molecules with identical chemical tables
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I have a few comments that might streamline some of the stuff you're doing here. Interoperability work is unfortunately a catastrophic mess of complexity, so "unit" tests can't be the beautiful <10 LOC tests they teach in textbooks.
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
openff/interchange/_tests/unit_tests/interop/lammps/export/test_lammps.py
Outdated
Show resolved
Hide resolved
Incorporated suggestions for unique mol ID unit test improvements Co-authored-by: Matt Thompson <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…for molecule_hash
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Alright @mattwthompson, I've incorporated the streamlining suggestions you made in one form or another and verified that the test still behaves as expected. Could you have a final look through to see if we're ready to merge? |
@timbernat Thanks for your patience on this; since the actual change is fairly small I'm just going to pull the trigger If there are other changes that would make your work run smoother, please feel free to coordinate with me on them or open PRs directly. Stuff in |
Thanks so much for all the help, @mattwthompson! I've deleted the A final question, what should be the fate of the |
Let me try to revive it and see if it needs a rebase or something funkier |
It looks like that branch wasn't so out of date it couldn't have |
Description
Follow-up to recommendations made in Issue 999
Checklist